fix: prevent empty history corruption during cancel/resume operations #8160
Conversation
- Make resume tolerant of empty API conversation history - Standardize reads using helper functions instead of direct JSON.parse - Add transaction helper for safe read-modify-write operations - Guard against unintentional empty writes with allowEmpty flag This fixes the issue where canceling during model's thinking/streaming phase could leave the API history file as an empty array ([]), causing the chat to lock when resuming. Fixes #8153 and #5333
dosubot
bot
added
size:M
hannesrudolph
added
the
Issue/PR - Triage
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| // Commit the changes | ||
| await saveApiMessages({ messages: updatedMessages, taskId, globalStoragePath }) | ||
|
|
||
| return updatedMessages |
There was a problem hiding this comment.
While this transaction helper is a good step forward, there's still a small race condition window between the read and write operations. Have you considered implementing file locking or using a more robust transaction mechanism like a write-ahead log? This could provide stronger guarantees against concurrent modifications.
| ) | ||
| // Initialize with empty arrays to allow the task to continue | ||
| modifiedApiConversationHistory = [] | ||
| modifiedOldUserContent = [] |
There was a problem hiding this comment.
The empty history fallback is solid defensive programming. Could we enhance this with a recovery mechanism that checks for backup files? Something like:
| modifiedOldUserContent = [] | |
| if (apiHistory.length === 0) { | |
| // Check for backup file first | |
| const backupPath = `${this.apiConversationHistoryFilePath}.backup`; | |
| if (await fileExists(backupPath)) { | |
| apiHistory = await readApiMessages(backupPath); | |
| this.say("info", "Recovered conversation from backup"); | |
| } else { | |
| this.say("info", "API conversation history is empty, starting with minimal resumption"); | |
| } | |
| } |
| // Guard against unintentional empty writes | ||
| if (updatedMessages.length === 0 && currentMessages.length > 0 && !options.allowEmpty) { | ||
| console.warn( | ||
| `[transactApiMessages] Preventing empty write for taskId: ${taskId}. Current has ${currentMessages.length} messages. Use allowEmpty: true to force.`, |
There was a problem hiding this comment.
I notice we're using different log levels ('warn' vs 'debug') for similar operations. Would it make sense to standardize these? For example, all successful operations could use 'debug' while warnings/errors use 'warn' or 'error'.
| * @param options - Optional configuration | ||
| * @returns The updated messages | ||
| */ | ||
| export async function transactApiMessages({ |
There was a problem hiding this comment.
This transactApiMessages function would benefit from comprehensive JSDoc comments explaining the transaction semantics and the guarantees it provides. The current JSDoc is good but could be expanded to mention the race condition limitation:
| export async function transactApiMessages({ | |
| /** | |
| * Transaction helper for safe read-modify-write operations on API messages. | |
| * Ensures atomic updates by reading, modifying, and writing under a conceptual lock. | |
| * | |
| * Note: This provides advisory locking but not true atomicity. | |
| * A small race condition window exists between read and write. | |
| * | |
| * @param taskId - The task ID | |
| * @param globalStoragePath - The global storage path | |
| * @param updater - A pure function that takes the current messages and returns the updated messages | |
| * @param options - Optional configuration | |
| * @returns The updated messages | |
| */ |
| // Allow empty writes for edit/delete operations | ||
| await currentCline.overwriteApiConversationHistory( | ||
| currentCline.apiConversationHistory.slice(0, apiConversationHistoryIndex), | ||
| true, // allowEmpty: true for edit/delete operations |
There was a problem hiding this comment.
Good use of the allowEmpty flag here for intentional deletions. Consider adding an inline comment explaining why this specific operation needs to allow empty writes, to help future maintainers understand the design decision.
…nversationHistory
dosubot
bot
added
size:L
Summary
This PR fixes the issue where canceling during a model's thinking/streaming phase could leave the API history file as an empty array ([]), causing the chat to lock when resuming.
Problem
As detailed by @hannesrudolph in #8153, the root cause is a race condition where:
Solution
Implemented the 4-part fix proposed by @hannesrudolph:
1. ✅ Make resume tolerant of empty history
2. ✅ Standardize reads with helper functions
3. ✅ Add transaction helper for safe read-modify-write
4. ✅ Guard empty writes with allowEmpty flag
Testing
Impact
Review Confidence
Code review completed with 95% confidence - all proposed fixes correctly implemented.
cc @hannesrudolph for review
Important
Fixes issue of chat lock due to empty history by making resume tolerant of empty history, standardizing JSON reads, and adding safe transaction operations.
Task.ts: ModifyoverwriteApiConversationHistory()to prevent empty writes unlessallowEmptyis true.ClineProvider.ts: Update to handle empty history gracefully and allow empty writes for edit operations.apiMessages.ts: AddtransactApiMessages()for atomic read-modify-write operations.ClineProvider.spec.ts,webviewMessageHandler.delete.spec.ts, andwebviewMessageHandler.edit.spec.tsto validate new behavior.ClineProvider.tsusingreadApiMessages().Task.ts.This description was created by
for 3caf3e9. You can customize this summary. It will automatically update as commits are pushed.